Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Reviews to Subjects #597

Open
wants to merge 7 commits into
base: add-shoulda-matchers-gem
Choose a base branch
from

Conversation

PaoloCappelli
Copy link
Contributor

@PaoloCappelli PaoloCappelli commented Nov 19, 2024

Purpose

Add the ability to submit 1-5 stars reviews to a Subject.

Summary

  • Create new Review model that for each User and Subject will store a rating
  • Add average_rating column to Subject to store the average of all the reviews associated with it
  • Add logic to update the Subject's average_rating every time a Review associated with it changes
  • Add ReviewsController
  • Update the Subjects view to show and submit reviews for a particular subject

Guidance 👀

Before adding any system specs to test the new UI. I would like to know your thoughts on the current approach, what's missing and what's not necessary
##Demo

Screen.Recording.2024-11-20.at.6.57.59.PM.mov

@PaoloCappelli PaoloCappelli force-pushed the add-reviews-to-subjects branch 5 times, most recently from 7436f62 to fdd05ad Compare November 20, 2024 02:15
@PaoloCappelli PaoloCappelli marked this pull request as draft November 20, 2024 02:17
@PaoloCappelli PaoloCappelli force-pushed the add-reviews-to-subjects branch 2 times, most recently from dfc5c9b to d4cb4f7 Compare November 20, 2024 14:49
create_table :reviews do |t|
t.references :user, null: false, foreign_key: true
t.references :subject, null: false, foreign_key: true
t.integer :rating
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this not null?

Comment on lines +5 to +15
def create
@review = current_user.reviews.create!(subject_id: params[:subject_id], rating: params[:rating])

redirect_to subject_path(@review.subject)
end

def update
@review.update!(rating: params[:rating])

redirect_to subject_path(@review.subject)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about combining these two actions and doing an "update or create"?

Comment on lines +23 to +30
<%= link_to new_user_session_path do %>
<div class="login-container">
Inicia sesión para calificar
<span class="material-symbols-outlined">
login
</span>
</div>
<% end %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we show the same stars, but when an unauthenticated user clicks them we redirect them to log in?

@@ -0,0 +1,5 @@
class AddAverageRatingToSubjects < ActiveRecord::Migration[7.1]
def change
add_column :subjects, :average_rating, :float
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm ok with adding this, but i wonder if we should start with calculating it on the fly

</div>
<% if current_user %>
<div class="rating-item">
<% for value in 1..5 %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<% for value in 1..5 %>
<% 1.upto(5).each |value| %>

<div class="rating-container">
<div class="rating-item">
<button class="material-icons mdc-icon-button">star</button>
<%= @subject.average_rating.present? ? @subject.average_rating : "Sin calificar" %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<%= @subject.average_rating.present? ? @subject.average_rating : "Sin calificar" %>
<%= @subject.average_rating || "Sin calificar" %>

transition: transform 0.25s ease;

&:hover {
transform: scale(1.3);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it easy/worth it highlighting all the stars to the left of the one that is on hover?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants